Replace haskell-src-exts with ghc-lib-parser - #39
Conversation
brandonchinn178
left a comment
There was a problem hiding this comment.
Cursory skim looks fine to me; AI is usually pretty good at mechanical transformations like convertExpr
| [ OverloadedStrings, | ||
| OverloadedRecordDot, | ||
| TupleSections, | ||
| LambdaCase, | ||
| MultiWayIf, | ||
| PostfixOperators, | ||
| QuasiQuotes, | ||
| UnicodeSyntax, | ||
| MagicHash, | ||
| ForeignFunctionInterface, | ||
| TemplateHaskell, | ||
| RankNTypes, | ||
| MultiParamTypeClasses, | ||
| RecursiveDo, | ||
| TypeApplications | ||
| ] |
There was a problem hiding this comment.
You might be able to get the currently active extensions in the QuasiQuoter with extsEnabled and pass them through to here
There was a problem hiding this comment.
Oooh I wasn't aware that was possible. Thanks a lot, this is a big improvement! Pushed this in 1f6dc0e.
I'll take the liberty of renaming some files and functions and moving them into the "LanguageHaskell" folder after we're happy, but am keeping those changes to the end of this PR to facilitate review.
| convertExpr (HsProjection _ flds) = | ||
| Right (TH.ProjectionE (fmap (\(L _ (DotFieldOcc _ (L _ fld))) -> fieldLabelToString fld) flds)) | ||
| #endif | ||
| convertExpr _ = Left "Unsupported Haskell expression form in hpgsql's SQL quasi-quoter" |
There was a problem hiding this comment.
Nit: I'd recommend explicitly enumerating instead of wildcard so that when new expressions are added, you'll get alerted to it and decide if you want to support it or not
There was a problem hiding this comment.
I hope bug reports will come with the expression that failed to be parsed, but I followed your suggestion and found that the error messages we can show are much nicer when we list every language feature. Plus it really helped me see what is still unsupported, and led me to support a few more cases (and add tests as I go).
ea6c015 is a pretty good summary of what we still don't support, and I'm planning on adding support for at least let and do.
What else in that list do you think is worth supporting?
There was a problem hiding this comment.
Yes, that's another good reason to explicitly list out branches - it forces you to make a decision whether you want to support something or not
I'd think all the TH constructs could be represented with ghc-lib constructs. Is it just a matter of doing the work or not? If so, I think what you have is fine for now, and you can always implement incrementally over time if people ask
There was a problem hiding this comment.
It's just a matter of doing the work and maintaining it, I guess. But it would be nice to keep it relatively small - it's not that little in CPP macros and LOC, and it's not hard to float out expressions from the quasiquoter for users when necessary if they're "exotic".
"Exotic" is subjective, of course, but you and I agree on OverloadedRecordDot being nice to have, and I'm sure there's a lot more users would agree on since I expect most #{} and ^{} to be small Haskell expressions.
There was a problem hiding this comment.
Yep, makes sense to me. Maybe in the error message, add some detail around "define outside quasiquoter, raise an issue at github if support should be added"?
| someNewThExtension -> Map.lookup (show someNewThExtension) allGhcLibParserExtensions | ||
|
|
||
| allGhcLibParserExtensions :: Map String Extension | ||
| allGhcLibParserExtensions = Map.fromList $ map (\ex -> (show ex, ex)) [minBound..maxBound] | ||
|
|
There was a problem hiding this comment.
Interesting strategy! I think I like it! It does make an assumption that the show implementation of the TH extension matches the show implementation of the ghc-lib-parser extension, but I think that's a reasonable assumption. Maybe just document it in the comment?
| @@ -0,0 +1,322 @@ | |||
| {-# LANGUAGE CPP #-} | |||
| {-# LANGUAGE PackageImports #-} | |||
| {- FOURMOLU_DISABLE -} | |||
There was a problem hiding this comment.
Why use fourmolu if you just disable entire files 😅
There was a problem hiding this comment.
Fourmolu is choking on some of the files with CPP macros :(
hpgsql/src/Hpgsql/GhcParseExp.hs:173:9-10
The GHC parser (in Haddock mode) failed:
[GHC-58481] parse error on input `<-'
I read their github, and it seems support for CPP macros is quite limited.
I was able to narrow the range of lines of code with fourmolu disabled in FromThExtension.hs, but in this file failed after a few attempts, so left the entire file unformatted because it didn't feel worth the trouble.
But I'm not very knowledgeable of fourmolu. Do you know if there's a better way of doing this?
There was a problem hiding this comment.
It's fine for the extensions file, since it's basically one giant pattern match. But this file is a bit more involved. I would recommend just as much as possible, break out CPP into isolated functions and only disable fourmolu for that function as a whole, instead of the entire file.
|
|
||
| -- Now come our list of unsupported language features | ||
| #if MIN_VERSION_ghc_lib_parser(9,10,0) | ||
| convertExpr (HsEmbTy {}) = unsupportedLanguageFeatureMsg "Embedded type expressions are" |
There was a problem hiding this comment.
Nit: If you just reword this as "___ expressions", you could do name ++ " expressions are unsupported in hgpsql ..."? So the argument becomes "label of expression construct" instead of "arbitrary prefix with the correct grammar to match the message"
| convertPat (AsPat _ (L _ rdr) _ (L _ p)) = TH.AsP (rdrToName rdr) <$> convertPat p | ||
| #endif | ||
| convertPat (BangPat _ (L _ p)) = TH.BangP <$> convertPat p | ||
| convertPat _ = Left "Unsupported pattern form in hpgsql's SQL quasi-quoter. Please file a bug report at https://github.com/mzabani/hpgsql/issues if you want this." |
There was a problem hiding this comment.
The only wildcard matches now are on lists, but every constructor has been explicitly laid out.
| convertHsLit (HsWordPrim _ w) = Right (TH.WordPrimL w) | ||
| convertHsLit (HsFloatPrim _ fl) = Right (TH.FloatPrimL (rationalFromFractionalLit fl)) -- TODO Why rational? | ||
| convertHsLit (HsDoublePrim _ fl) = Right (TH.DoublePrimL (rationalFromFractionalLit fl)) | ||
| convertHsLit _ = Left "Unsupported literal type in SQL quasi-quoter" |
| convertType t | ||
| convertType (HsQualTy _ _ (L _ t)) = | ||
| convertType t | ||
| convertType _ = Left "Unsupported type in SQL quasi-quoter type signature" |
This is just Haskell syntax for data constructors checking the first character. I understand it now.
This should fix #36, but there's lots of learning and self-reviewing before I can merge this. See issue for more explanations.
TODO to implement and mark it as non-draft:
letanddoTODO after review and before merging: